Fix: #6087 Display specific error message for course slug collision (revision) #6168
Conversation
app/assets/javascripts/utils/api.js
Outdated
| @@ -321,6 +321,10 @@ const API = { | |||
| }); | |||
|
|
|||
| if (!response.ok) { | |||
| if (response.status === 422 || response.status === 400) { | |||
There was a problem hiding this comment.
Rather than special handling in JS, this should follow the pattern used elsewhere in the app where the message from the JSON error response shown, rather than a message that is part of frontend code.
See assignments_controller.rb for some examples of this pattern, or the PR that fixed another instance of this sort of issue: #1760
There was a problem hiding this comment.
@ragesoss I've made the changes you can check them. Now, the backend dynamically returns JSON error responses in all supported languages based on the selected language. I’ve also updated the screenshots for your review. Let me know if you have any feedback!
|
Just a suggestion, Displaying the existing course slug as a clickable link opening that existing course in a new tab so that users can easily identify which course it is colliding with by clicking on it. This will enhance clarity and user experience. |
It can be implemented, but the primary goal is to provide a clear and accurate message so that the user can choose a different slug. |
app/assets/javascripts/utils/api.js
Outdated
| @@ -321,6 +321,11 @@ const API = { | |||
| }); | |||
|
|
|||
| if (!response.ok) { | |||
| if (response.status === 422 ) { | |||
There was a problem hiding this comment.
Is this part of the change necessary? For other errors where we display the backend message, the existing data flows work without special handling of a specific status.
There was a problem hiding this comment.
A 422 response returns a JSON structure (data.errors) with a dynamically generated slug, while other responses return plain text. If special handle is not applied, it results in an empty object, failing to extract the error message and causing the default Sentry log error (e.g., "Sentry is not defined")
There was a problem hiding this comment.
Other cases that use this error message handling pattern don't require extra code for handling the specific status. Can you identify what is different about this, compared with (for example) #createRandomPeerAssignments? I'd like for the error handling flow to be as simple and uniform as possible, so that most errors rely on the same code pathways without introducing new frontend code for each new case that needs user-readable error messages.
There was a problem hiding this comment.
if i remove special handle this happens it throws empty [] this becomes default message of sentry
There was a problem hiding this comment.
@ragesoss The code is working perfectly after removing the special Handle logic and adding a try-catch block around Sentry.captureMessage to handle failures. Unnecessary if conditions have also been removed. Please let me know if you need any more refinements.
| @@ -51,6 +51,11 @@ def update | |||
| ensure_passcode_set | |||
| UpdateCourseWorker.schedule_edits(course: @course, editing_user: current_user) | |||
| render json: { course: @course } | |||
| rescue Course::DuplicateCourseSlugError => e | |||
| message = I18n.t('courses.error.duplicate_slug', slug: e.slug) | |||
There was a problem hiding this comment.
This is not indented properly. I expect linting to fail.
There was a problem hiding this comment.
@ragesoss I have fixed the linting issues, ensuring proper formatting and readability. Let me know if you need any further improvements
There was a problem hiding this comment.
For your reference, I reviewed both files. I added an extra function to keep the lines within the 16-line limit, ensuring compliance with linting.
|
It looks like a test broke because of this new validation. You likely need to make a slight adjustment to that test so that it doesn't trigger the duplicate slug error. |
What this PR does
(Revision)
Fixes #6087
This PR addresses the issue where a generic "Internal Server Error" message was displayed when a user attempted to change a course slug to one that already existed.
Screenshots
Before:
This is an error occurring not showing proper message only showing Internal Server Error
Course titles can be identical for different organizations or institutions, but must be unique within the same organization or institution. This is because course titles are used to dynamically generate URLs (slugs), and duplicate titles within the same organization would create URL conflicts.
look: below
After:
Showing proper message
Updated